Make $idx/$len work for all array objects and not leave old values in contexts #113

rragan · 2012-08-15T16:57:32Z

Currently $idx/$len are set in the push method which is called by a number of places other than sections doing iterations. This results in $idx:undefined values getting created. Also $idx/$len are not propertly set when the section is an array of simple types (e.g. [1,2,3] versus an array of objects where it works OK.

This change moves the $idx/$len setting to the section method confining the creation of these values to just iteration sections. The old way also left $idx/$len values littering the context area (try a contextDump and look). This change removes them after the iteration completes.

Added two tests for $idx/$len on array of simple types. No external doc changes needed as I don't think the limitation of only working for arrays of objects was documented. It will just work on more things now and not surprise the user.

…h is called by parts of the code that have nothing to do with secton iteration to the section method in the code explicitly for iteration. After iteration is done, remove $idx and $len so they won't pollute the context data after exiting the iteration. All tests pass after this change. No external doc change as end user behavior remains the same (except for $idx/$len no longer showing up in dumpContext after the iteration completes)

…ues and not objects. This did not work with the previous way $idx/$len was implemented. All array objects should support $idx/$len now

travisbot · 2012-08-15T16:58:17Z

This pull request passes (merged 4606d03 into 7e3a258).

jairodemorais · 2012-08-15T17:55:21Z

lgtm @rragan , thanks you made a great work. let's wait the team's review. @vybs @jimmyhchan @iamleppert

jimmyhchan · 2012-08-15T18:37:54Z

Silly question: Can we have $idx work for every iterable type: objects, arrays, array of objects. Objects don't have a length property but aren't we already storing something via stack.index.

I'm not sure if this is what @vybs intended in the first place but having it for everything would be nice.

Could you please elaborate on what the delete section does. I'm not familiar with this section of the code but it looks like we are doing some manual garbage collection?

vybs · 2012-08-15T20:07:19Z

test/jasmine-test/spec/grammarTests.js

@@ -70,6 +70,20 @@ var grammarTests = [
      message: "should test an array"
  },
  {
+     name:     "array",
+     source:   "{#names}({$idx}).{title} {.}{~n}{/names}",
+     context:  { title: "Sir", names: [ "Moe", "Larry", "Curly" ] },


please move this tests out of this file to the other one.

Not sure when this file was split up ( since I did not see a PR, @jairodemorais )

There is no grammar changes relates to $idx, hence

@rragan as Veena said, t

he tests that use helpers have to be in the helpersTests located on /dustjs-helpers/test/jasmine-test/spec.

Ahhh, the noob spotted tests using $idx and flocked the birds of a feather together. I guess there weren’t $idx tests in the other file that I noticed? I’ll move these and can add others there at the same time. OK?

Rich

From: Veena Basavaraj [mailto:notifications@github.com]
Sent: Wednesday, August 15, 2012 1:07 PM
To: linkedin/dustjs
Cc: Ragan, Richard
Subject: Re: [dustjs] Make $idx/$len work for all array objects and not leave old values in contexts (#113)

In test/jasmine-test/spec/grammarTests.js:

@@ -70,6 +70,20 @@ var grammarTests = [

message: "should test an array"

},

{

name: "array",

source: "{#names}({$idx}).{title} {.}{~n}{/names}",

context: { title: "Sir", names: [ "Moe", "Larry", "Curly" ] },

please move this tests out of this file to the other one.

Not sure when this file was split up ( since I did not see a PR, @jairodemoraishttps://github.com/jairodemorais )

There is no grammar changes relates to $idx, hence

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/113/files#r1386643.

rragan · 2012-08-15T21:07:05Z

The implementation defines these for anything that the existing code defines index and len values so I don’t think it’s missing any cases. The Stack function defines these two property values and the push propagates their current values but doesn’t change them. Chunk.prototype.section defines them for the iteration using push. That’s the only uses I can see.

The previous code did this
context.stack.head['$len'] = len;
in the push method. It just adds a new property to the current context named $len (and similarly for $idx). Since the property is never removed, a contextDump will show you leftover $idx/$len values at contexts where an iteration was done. Not super harmful but it would allow referencing the last value of $idx and $len after the iteration. To my mind, these value ought to go out of scope once the loop exits.
The added code is just:
delete context.stack.head['$idx'];
delete context.stack.head['$len'];
to remove these two inserted values from the iteration context. Not really garbage collection so much as putting things back like we found them.

Note that this feature creates reserved names of $idx and $len which can get stomped on if the user employs them for their own data values. I’m OK with that but it would be possible to save any existing user value name $idx/$len and restore it after the iteration scope. I don’t think this is worthwhile. Should all $xxx names be reserved for the implementation?

Rich

From: jimmyhchan [mailto:notifications@github.com]
Sent: Wednesday, August 15, 2012 11:38 AM
To: linkedin/dustjs
Cc: Ragan, Richard
Subject: Re: [dustjs] Make $idx/$len work for all array objects and not leave old values in contexts (#113)

Silly question: Can we have $idx work for every iterable type: objects, arrays, array of objects. Objects don't have a length property but aren't we already storing something via stack.index.

I'm not sure if this is what @vybshttps://github.com/vybs intended in the first place but having it for everything would be nice.

Could you please elaborate on what the delete section does. I'm not familiar with this section of the code but it looks like we are doing some manual garbage collection?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/113#issuecomment-7764864.

vybs · 2012-08-15T21:18:57Z

test/jasmine-test/spec/grammarTests.js

+     message: "should test an array with non-object elements using $idx"
+   },
+  {
+      name:     "array",


@jairodemorais if @rragan is ok, a few more test cases makes sense here

{#.} {$idx} {/.}

more edge cases like these

empty array,

empty map

Happy to add more. I’ll put on my “Got Tests” hat – which I really do have.

Am I correct in understanding the two existing $idx/$len tests don't belong in the grammar tests and should also be moved?

Rich

From: Veena Basavaraj [mailto:notifications@github.com]
Sent: Wednesday, August 15, 2012 2:23 PM
To: linkedin/dustjs
Cc: Ragan, Richard
Subject: Re: [dustjs] Make $idx/$len work for all array objects and not leave old values in contexts (#113)

In test/jasmine-test/spec/grammarTests.js:

@@ -70,6 +70,20 @@ var grammarTests = [

message: "should test an array"

},

{

name: "array",

source: "{#names}({$idx}).{title} {.}{~n}{/names}",

context: { title: "Sir", names: [ "Moe", "Larry", "Curly" ] },

expected: "(0).Sir Moe\n(1).Sir Larry\n(2).Sir Curly\n",

message: "should test an array with non-object elements using $idx"

},

{

name: "array",

@jairodemoraishttps://github.com/jairodemorais if @rraganhttps://github.com/rragan is ok, a few more test cases makes sense here

{#.} {$idx} {/.}

more edge cases like these

empty array,

empty map

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/113/files#r1387479.

…ersTests from grammarTests; add more tests for idx/len cases

travisbot · 2012-08-21T00:03:14Z

This pull request passes (merged c6e46e3 into 7e3a258).

travisbot · 2012-08-21T00:20:24Z

This pull request passes (merged dfc44de into 7e3a258).

rragan · 2012-08-21T00:27:58Z

Changes in response to previous PR:

Changed from delete to setting to undefined
Fixed case where $idx/$len were not defined on a section with just a single value
Moved tests for $idx/$len from grammarTests to helpersTests
Added more $idx/$len tests for single element case, empty map case
Changed source of dust for running dustjs-helpers/test/jasmine-test/server/specRunner.js

Ran main dust jasmine and client tests. Ran dustjs-helper server and client tests. All pass.

vybs · 2012-08-21T14:18:39Z

lgtm! thanks @rragan

one more test case to add:

one more test case

{#.}

{$idx} {$len}

{/.}

Also to make sure the above works for both lists of primitives ( i.e isArray ) and lists of objects

vybs · 2012-08-21T14:19:36Z

test/jasmine-test/spec/grammarTests.js

-      source:   "{#names}Size=({$len}).{title} {name}{~n}{/names}",
-      context:  { title: "Sir", names: [ { name: "Moe" }, { name: "Larry" }, { name: "Curly" } ] },
-      expected: "Size=(3).Sir Moe\nSize=(3).Sir Larry\nSize=(3).Sir Curly\n",
-      message: "should test an array"


one more test case

{#.}

{$idx}

{/.}

travisbot · 2012-08-21T16:24:58Z

This pull request passes (merged 54f29ae into 7e3a258).

rragan · 2012-08-21T16:29:05Z

Reverted line of code for loading dustjs-linkedin in dustjs helpers specRunner.js and added a test for {#.} {$ids} {/.} use case to address outstanding comments.

vybs · 2012-08-21T16:33:18Z

@rragan , does $idx also work within a helper inside a block. Just double checking if we have tests for this?

{#a}

{#helper index=$idx/}

{/a}

rragan · 2012-08-21T16:39:32Z

There is already an existing test for this:
{
name: "if helper using $idx",
source: '{#list}{@if cond="( {$idx} == 1 )"}

{y}

{/if}{/list}',
context: { x : 1, list: [ { y: 'foo' }, { y: 'bar'} ]},
expected: "

bar

",
message: "should test the if helper using $idx"
},

vybs · 2012-08-21T16:42:34Z

great, one last test case, hope I am not been too fussy!

a test case for lists within lists, I know this will work:)

{#a} {#b} {#c } {$idx} {/c} {/b} {/a}

Second, if say a is not a list, still dust allows us to do {#a} {/a}, so what does $idx and $len evaluate to?

travisbot · 2012-08-21T18:25:27Z

This pull request passes (merged 0f0bdf9 into 7e3a258).

rragan · 2012-08-21T18:26:22Z

Test added for $idx with nested loops and tests the value before and after the inner loop to be sure inner loop does not affect outer loop iteration count

vybs · 2012-08-21T18:28:39Z

Love it. Merging it!

One more request ( can we add somehow link this ticket from the wiki, for all the extension we have done, sometimes folks may want to read the reason behind why we added a new feature )

rragan · 2012-08-21T18:28:39Z

Re "Second, if say a is not a list, still dust allows us to do {#a} {/a}, so what does $idx and $len evaluate to?"

if a is empty/non-existent, nothing gets output so value of $idx/$len is moot.

if a has one element but is not a list, dust defines that the body will be output. In that case $len=1 and $idx=0 within the body of the single "iteration". There is a test for this already.

Make $idx/$len work for all array objects and not leave old values in contexts

rragan added 2 commits August 14, 2012 13:23

Add tests for $idx and $len when iteration object contains simple val…

4606d03

…ues and not objects. This did not work with the previous way $idx/$len was implemented. All array objects should support $idx/$len now

jairodemorais mentioned this pull request Aug 15, 2012

supporting $idx and $len in list of primitives #110

Closed

vybs reviewed Aug 15, 2012
View reviewed changes

Revise idx/len changes to handle one element case; move tests to help…

c6e46e3

…ersTests from grammarTests; add more tests for idx/len cases

Restore tapper tests that got lost

dfc44de

vybs reviewed Aug 21, 2012
View reviewed changes

Revert to loading dustjs way done originally. Add test for {#.} case

54f29ae

Add test for idx in nested loops

0f0bdf9

vybs added a commit that referenced this pull request Aug 21, 2012

Merge pull request #113 from rragan/master

d2be041

Make $idx/$len work for all array objects and not leave old values in contexts

vybs merged commit d2be041 into linkedin:master Aug 21, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make $idx/$len work for all array objects and not leave old values in contexts #113

Make $idx/$len work for all array objects and not leave old values in contexts #113

rragan commented Aug 15, 2012

travisbot commented Aug 15, 2012

jairodemorais commented Aug 15, 2012

jimmyhchan commented Aug 15, 2012

vybs Aug 15, 2012

jairodemorais Aug 15, 2012

rragan Aug 15, 2012

rragan commented Aug 15, 2012

vybs Aug 15, 2012

rragan Aug 15, 2012

travisbot commented Aug 21, 2012

travisbot commented Aug 21, 2012

rragan commented Aug 21, 2012

vybs commented Aug 21, 2012

vybs Aug 21, 2012

travisbot commented Aug 21, 2012

rragan commented Aug 21, 2012

vybs commented Aug 21, 2012

rragan commented Aug 21, 2012

vybs commented Aug 21, 2012

travisbot commented Aug 21, 2012

rragan commented Aug 21, 2012

vybs commented Aug 21, 2012

rragan commented Aug 21, 2012

Make $idx/$len work for all array objects and not leave old values in contexts #113

Make $idx/$len work for all array objects and not leave old values in contexts #113

Conversation

rragan commented Aug 15, 2012

travisbot commented Aug 15, 2012

jairodemorais commented Aug 15, 2012

jimmyhchan commented Aug 15, 2012

vybs Aug 15, 2012

Choose a reason for hiding this comment

jairodemorais Aug 15, 2012

Choose a reason for hiding this comment

rragan Aug 15, 2012

Choose a reason for hiding this comment

rragan commented Aug 15, 2012

vybs Aug 15, 2012

Choose a reason for hiding this comment

rragan Aug 15, 2012

Choose a reason for hiding this comment

travisbot commented Aug 21, 2012

travisbot commented Aug 21, 2012

rragan commented Aug 21, 2012

vybs commented Aug 21, 2012

vybs Aug 21, 2012

Choose a reason for hiding this comment

travisbot commented Aug 21, 2012

rragan commented Aug 21, 2012

vybs commented Aug 21, 2012

rragan commented Aug 21, 2012

vybs commented Aug 21, 2012

travisbot commented Aug 21, 2012

rragan commented Aug 21, 2012

vybs commented Aug 21, 2012

rragan commented Aug 21, 2012